Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Typesafe IPC #331

Merged
merged 8 commits into from
Jun 22, 2024
Merged

Typesafe IPC #331

merged 8 commits into from
Jun 22, 2024

Conversation

rkuklik
Copy link
Contributor

@rkuklik rkuklik commented Jun 18, 2024

This is first (and probably not last) PR for IPC refactor. For now this is a draft because I am unsure if PRs of this size are either to big or too inconsequential.

Apart from sending and receiving itself, I will take a look at some semi-duplicated code (looking at you, RequestSend and RequestRecv), though this will not be included in this PR.

Is there any form of compatibility guarantee between swww and swww-daemon if version bump ocuurs?

@LGFae
Copy link
Owner

LGFae commented Jun 18, 2024

Is there any form of compatibility guarantee between swww and swww-daemon if version bump ocuurs?

No. You may break as many things as you want :)

@rkuklik
Copy link
Contributor Author

rkuklik commented Jun 19, 2024

I noticed couple cfg(target_os = "linux") in mmap module. Is there a reason for this? As far as I know, wayland is official only for linux. Does this project support (or plans to support) BSDs?

@rkuklik
Copy link
Contributor Author

rkuklik commented Jun 19, 2024

This should conclude first part if IPC refactor. In next PR I will reorganize daemon and client a bit to facilitate easier adjustments.

@rkuklik rkuklik marked this pull request as ready for review June 19, 2024 13:04
@LGFae
Copy link
Owner

LGFae commented Jun 22, 2024

I noticed couple cfg(target_os = "linux") in mmap module. Is there a reason for this? As far as I know, wayland is official only for linux. Does this project support (or plans to support) BSDs?

Wayland is already being used by some people in FreeBSD. They've even opened issues in this project.

Furthermore, in general, Wayland isn't Linux-specific. There was a time where libwayland made Linux-specific syscalls in its implementation, but it's no longer the case.

Copy link
Owner

@LGFae LGFae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it took me so long. This is a large PR and I am kind of busy right now.

In any case, I am really liking how this is turning out. I just have a tiny nitpick regarding how you do imports.

Comment on lines +1 to +12
use std::env;
use std::marker::PhantomData;
use std::sync::OnceLock;
use std::time::Duration;

use rustix::fd::OwnedFd;
use rustix::io::Errno;
use rustix::net;

use super::ErrnoExt;
use super::IpcError;
use super::IpcErrorKind;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why you are doing imports like this? I don't particularly mind, but it isn't consistent with the rest of the codebase. I supposed we could normalize them later...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I simply prefer this style. When I change imports, the resulting diff is on one line (with entire path). Also, I think it is easier to parse (entire path in one spot, I don't have to jump around to see who declared something like fs module) and edit in general (dd in vim).

@rkuklik
Copy link
Contributor Author

rkuklik commented Jun 22, 2024

Wayland is already being used by some people in FreeBSD. They've even opened issues in this project.

Alright. And as for MmapedStr and MmapedBytes, is there a reason for their existence? I tried to look up their usage and it seems to me that they are only really used for IPC in daemon (to avoid lifetimes?). I am not that familiar with shared memory, but I would like to get rid of them in favor of regular references when I rework serde. Sounds OK?

@LGFae
Copy link
Owner

LGFae commented Jun 22, 2024

And as for MmapedStr and MmapedBytes, is there a reason for their existence? I tried to look up their usage and it seems to me that they are only really used for IPC in daemon (to avoid lifetimes?). I am not that familiar with shared memory, but I would like to get rid of them in favor of regular references when I rework serde. Sounds OK?

They are there to avoid copies. By memory mapping them we have only one shared memory buffer between the client and the daemon. Also, we won't be using serde, at all. Serde does a bunch of unnecessary copies around, and increases our dependencies significantly due to everything we have to pull. It's really good for large applications where writing everything manually would be a pain, or simply when you don't want to bother. But I went to great lengths to make IPC as efficient as I could. Our serialization and de-serialization will remain all manual.

I believe the easiest way of making the code more readable would be by using something like a builder pattern. You wrap RawMsg with a ReqBuilder. And then you can call builder.next_mmaped_str(), which will do all the "ugly" stuff for you.

@LGFae LGFae merged commit 90e205e into LGFae:main Jun 22, 2024
9 of 10 checks passed
@rkuklik
Copy link
Contributor Author

rkuklik commented Jun 22, 2024

Sorry, that's not really what I meant. By serde I meant reading and writing messages to the shared buffer (without a protocol). But when receiving and deserializing data, I noticed tho two aforementioned types being built. Is there a reason not to create &[u8] and &str directly and shove a lifetime parameter to relevant data structures? I didn't want to introduce a new dependency.

EDIT:
I just didn't understand the dedicated types. Also perhaps their removal could simplify logic in Mmap with unmap bool parameter (but not sure, didn't read enough of the relevant code)

@LGFae
Copy link
Owner

LGFae commented Jun 22, 2024

Ah, I get it now, my bad.

I don't 100% remember what the problem was, but I believe it was that, when we send in an animation, we will have the image bytes and the animation bytes all mapped, and I wanted to unmap just the image bytes and keep the animation bytes.

In any case, if you think you can simplify the logic by removing them, go ahead and give it a try!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants